Skip to content

Allow case-insensitive ok or OK replies from pluggable discoveries and monitors #1633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jan 24, 2022
Merged

Allow case-insensitive ok or OK replies from pluggable discoveries and monitors #1633

merged 3 commits into from
Jan 24, 2022

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Jan 24, 2022

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?
    Allow case-insensitive ok or OK replies from pluggable discoveries and monitors.

What is the current behavior?
If a discovery or a monitor replies to a command, for example, with:

{
  "eventType": "describe",
  "message": "ok",
  "port_description": {
    "protocol": "serial",
    "configuration_parameters": {
    }
  }
}

the answer is treated as an error because it requires a case-sensitive "OK".
Also. the error message is completely useless:

Error getting port settings details: Port monitor error: command 'describe' failed: ok

What is the new behavior?
The response is accepted for any case-insensitive ok / OK

Does this PR introduce a breaking change, and is
titled accordingly?
No breaking changes

@cmaglie cmaglie self-assigned this Jan 24, 2022
@cmaglie cmaglie requested a review from silvanocerza January 24, 2022 10:47
@PaulStoffregen
Copy link

Looks like eventType parsing is also case sensitive. For example, if I reply with this JSON

{
"eventType": "DESCRIBE",
"message": "OK",
"port_description": {
"protocol": "Teensy",
"configuration_parameters": {
}
}
}

I get this error, which isn't nearly as difficult as the OK issue, since it tells me what it expected.

Error getting port settings details: Port monitor error: communication out of sync, expected 'describe', received 'DESCRIBE'

@cmaglie
Copy link
Member Author

cmaglie commented Jan 24, 2022

I get this error, which isn't nearly as difficult as the OK issue, since it tells me what it expected.

good point, I've improved the error messages in the same way.

@cmaglie cmaglie force-pushed the disc_and_mon_less_strict branch from 98a5e24 to 87665c9 Compare January 24, 2022 13:17
@per1234 per1234 added topic: code Related to content of the project itself type: enhancement Proposed improvement labels Jan 24, 2022
@cmaglie cmaglie merged commit 5f8cb4b into arduino:master Jan 24, 2022
@cmaglie cmaglie deleted the disc_and_mon_less_strict branch January 24, 2022 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silvanocerza silvanocerza silvanocerza approved these changes

Assignees

@cmaglie cmaglie

Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cmaglie @PaulStoffregen @silvanocerza @per1234